feat(github): render live row-copy progress on sharded table lines - #1191

Pull request overview

This PR enhances sharded apply PR-comment UX by aggregating per-shard row-copy metrics (rows copied/total + ETA) into the per-table rollup line, so operators can see live copy progress at the table level instead of only a generic state phrase.

Changes:

• Aggregate RowsCopied, RowsTotal, and ETASeconds across shard tasks when building sharded apply template data.
• Render a per-table progress bar + rows/ETA line for actively copying sharded tables (keeping non-copy phases on state phrases).
• Add/adjust previews and tests to cover the new rendering and edge cases (starting copy, estimate exceeded, non-copy phases).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

🤖 Approved on Morgan's behalf by his AI agent.

Comment-rendering only, 37/37 green, and it reuses the single-deployment honesty guards (EstimateExceeded → "Finalizing copy", pct == 0 → "Starting copy...") rather than reinventing them. The division is safe — t.RowsTotal > 0 gates the whole branch — and shardTaskProgress replacing best wholesale rather than field-by-field is the right call, since it can't splice one task's status onto another's row counts.

Approving because this is strictly better than showing no progress at all. But two of the aggregated figures claim more than they can deliver once a rollout spans more than one dispatch wave, and I'd rather flag them now than have someone file them as bugs later.

1. The percentage regresses as waves start. RowsTotal sums only the shards that have reported. A 4-shard table with wave 1 in flight renders 500k / 1M = 50%; when wave 2 starts and adds its shard's total, the same table renders 500k / 2M = 25%. The bar goes backwards, in a comment that's edited in place, while the operator is watching. The doc comment frames the growing total as a feature — "live, not a plan-time promise" — which is a fair description of the mechanism but doesn't acknowledge that the rendered consequence is a progress bar that visibly loses ground.

2. The ETA is the slowest started shard, not the slowest shard. Unstarted shards contribute etaSeconds: 0, so max skips them entirely. During wave 1 of 4, the line reads as the table's ETA but is really wave 1's ETA — it will systematically under-report, and it under-reports most severely at the start, which is when people read it. "ETA 3 minutes" on a table with three waves still to dispatch is the kind of thing that costs the tool credibility.

Neither is hard to soften if you want them: denominating the percentage over all shards (treating unreported shards as unknown rather than zero), or suppressing the ETA line until every shard has reported, would both make the number mean what the label says. A cheaper option is to keep the figures and qualify the label while waves are outstanding — e.g. "ETA 3m (shards still dispatching)" — so the number stays useful without overclaiming.

Nit: the preview fixture bumps -40 from 45% to 62% alongside the new row figures. Worth a sanity check that the fixture's shard percent and the new RowsCopied/RowsTotal (914707/1466232 ≈ 62%) are meant to agree — they do here, which is a nice touch, but nothing enforces it, so a future edit to one can silently desync the preview from what the real renderer would produce.

🤖 Re-approved on Morgan's behalf by his AI agent, now at 91d28bcc. My earlier approval was on 496bd538; re-stamping so it covers the code that's actually here.

Reviewed the delta against the two things I raised.

ETA — fully fixed. ETA: ≥ 3m while ShardsReporting < len(Shards) is exactly right. The unstarted shards can only add rows and time, so a floor is the honest form, and ≥ communicates it without a sentence of explanation.

Fixture drift — fixed, and better than I suggested. Deriving previewMutesCopyPercent from the row constants means the summary line and rows line can't desync. Reused in the divergent fixture too.

Percentage regression — disclosed, not resolved. The rows line now says "across 1 of 4 shards", which gives an operator what they need to interpret the number. But the bar itself is unchanged:

pct := ui.RowCopyDisplayPercent(int(ui.ClampRows(t.RowsCopied, t.RowsTotal)*100/t.RowsTotal), t.RowsCopied)

RowsTotal still covers only reporting shards, so a 4-shard table still renders 62% on wave 1 and still drops to roughly 31% when wave 2 starts. The struct comment now says the rows line "discloses partial coverage rather than passing a wave's fraction off as the table's" — but the bar is the loudest element on the line and it is still the wave's fraction presented as the table's. The caveat sits on the quieter line beneath it.

Not blocking, because disclosure is a real improvement over silence and the alternative costs more than it's worth mid-review. If you want to close it later, qualifying the headline the same way the rows line is qualified would do it — e.g. 62% (1 of 4 shards), or holding the bar back until every shard reports and showing the phase word until then.

Two small things in the new code, neither blocking:

• writeShardedRowsAndETA's t.RowsTotal <= 0 early return is unreachable — writeShardedTableLine already gates the whole branch on t.RowsTotal > 0. Harmless as defensive code, just noting it isn't load-bearing.
• shardsReporting increments only when sp.rowsTotal > 0, while r.rowsCopied += sp.rowsCopied is unconditional. A shard reporting copied rows with a zero total would add to the numerator without the denominator or the count — which lands in EstimateExceeded → "Finalizing copy". That's a sane place to land, so I think it's fine; flagging in case that combination is supposed to be impossible rather than merely handled.

🤖 Re-approved on Morgan's behalf by his AI agent — his approval was at 91d28bcc and the head has moved to 2ff707ab. Re-verified rather than left to ride, and it's worth saying why: the new commit fixes a real bug that was present in the version he approved.

At 91d28bcc, shardedTableStatusesByKeyspace gated r.shardsReporting++ on sp.rowsTotal > 0 but accumulated the figures unconditionally:

if sp.rowsTotal > 0 {
    r.shardsReporting++
}
r.rowsCopied += sp.rowsCopied      // ran regardless
r.rowsTotal  += sp.rowsTotal

So a shard reporting copied rows before it had a row total contributed to the numerator without contributing to the denominator or to the coverage count — inflating the displayed percentage, and able to push it past 100% in the right ordering. 2ff707ab moves all four accumulations inside the same guard, so the numerator, denominator, ETA, and coverage count always describe the identical shard set. That's the correct invariant and the comment states it precisely.

The other two changes hold up as well:

• The headline now carries the coverage qualifier (62% (1 of 4 shards)), matching what the rows line already disclosed. Consistent, and it closes the gap where the bar could be read as the whole table's progress while later dispatch waves hadn't started.
• Dropping the if t.RowsTotal <= 0 { return } guard from writeShardedRowsAndETA is a consequence of the fix, not an oversight. Before, a zero total could coexist with copied rows, and the guard suppressed the line. Now a zero total means nothing is reporting at all — and in that state the coverage disclosure ("0 of 4 shards") is the informative thing to print, not something to hide.

CI green at the new head.